-
Notifications
You must be signed in to change notification settings - Fork 498
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add ability to send multiple requests at once #23
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is a first pass and will probably be altered before merging. Still needs more hooks and more tests. Only implemented for cURL so far. Trivia: I wrote this completely without internet access (and hence, no documentation either). Imagine my surprise when it worked first time.
If we end up with a situation where our hook doesn't fire for some reason, we shouldn't pass this on to the hooks. Instead, wait until the end when we ensure they're instances.
fsockopen doesn't support multiple simultaneous requests, so we just fake it here instead. What it actually does is to send every request sequentially, so the result back from Requests::request_multiple() will be the same.
curl.before_request and curl.after_request are now fired on every cURL request. curl.before_send and curl.after_send are for single requests, while curl.before_multi_add, curl.before_multi_exec and curl.after_multi_exec are for multiple requests.
curl_multi_info_read()'s 'result' value is the same as curl_errno(), which we handle per-request in a nicer way.
Rather than relying on typecasting the cURL resource to an integer to form a key, use the proper key instead.
To make it easier to track a request after it's complete, we pass in the original identifier along with the response.
This should all be abstracted away.
This is a bit of duplication (given that you can just make your own Requests_Hooks and add it), but it saves a few extra lines on the user's end.
This should fix the currently broken unit tests. Yay!
If we're in non-blocking mode, there's no reference to return. We're not really saving anything with this anyway.
This is currently lacking documentation. Coming soon.
multiple.request.complete is called on the request's hooking system, not the global one.
Also, move the callback to a proper method. Fixes 5.2 compatibility as well as avoiding duplication.
Previously, this checked a nonexistent variable, and I'm not sure why. If it ever needs to be fixed, blame me.
rmccue
added a commit
that referenced
this pull request
Oct 25, 2012
Add ability to send multiple requests at once
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Requests is currently missing a way to send multiple simultaneous requests. cURL has support for this natively, while fsockopen can't do it, but we can (and should) simply fake it for fsockopen.
This branch enables multiple request support, with faked support for fsockopen. At the moment, it's missing extensive tests, so it's not yet ready for merging.